Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't reparse parent elements when cloning. #1484

Open
wants to merge 8 commits into
base: sdf15
Choose a base branch
from
Open

Don't reparse parent elements when cloning. #1484

wants to merge 8 commits into from

Conversation

arjo129
Copy link

@arjo129 arjo129 commented Oct 1, 2024

🦟 Bug fix

Fixes #

Summary

Improves performance on sdf::Element::Clone by not requiring the parent elements to be reparsed. This was originally part of #1479. But it makes sense to seperate out the work.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

arjo129 and others added 3 commits September 6, 2024 07:42
@arjo129
Attempting to see if adding a cache will speed up the sdf::Root
0868abf 
construction

I'm just checking if adding a cache makes any sense.

Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129
Remove cacheing code. Make faster.
91228de 
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129
Style
0977059 
Signed-off-by: Arjo Chakravarty <[email protected]>
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Oct 1, 2024
@arjo129 arjo129 changed the title Arjo/fix/clone parent only Don't reparse parent elements when cloning. Oct 1, 2024
@scpeters
Copy link
Member

I think macOS build failures are due to osrf/homebrew-simulation#2834

@scpeters
Copy link
Member

I ran the PERFORMANCE_parser_urdf test several times with std_err redirected to /dev/null and see about a 18% reduction in average test time (1 - 0.928 / 1.133) ~= 0.18

sdf15

Ran the test 10 times:

Minimum Average Maximum
1.110 s 1.133 s 1.149 s 
$ time for i in $(seq 10); do bin/PERFORMANCE_parser_urdf 2> /dev/null ; done
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1103 ms)
[----------] 1 test from URDFParser (1103 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1103 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1123 ms)
[----------] 1 test from URDFParser (1123 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1123 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1107 ms)
[----------] 1 test from URDFParser (1107 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1108 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1149 ms)
[----------] 1 test from URDFParser (1149 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1149 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1117 ms)
[----------] 1 test from URDFParser (1117 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1117 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1100 ms)
[----------] 1 test from URDFParser (1100 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1100 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1109 ms)
[----------] 1 test from URDFParser (1109 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1109 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1112 ms)
[----------] 1 test from URDFParser (1112 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1112 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1116 ms)
[----------] 1 test from URDFParser (1116 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1116 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1118 ms)
[----------] 1 test from URDFParser (1118 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1118 ms total)
[  PASSED  ] 1 test.

real	0m11.335s
user	0m8.720s
sys	0m2.549s

this branch

Ran the test 10 times:

Minimum Average Maximum
0.890 s 0.928 s 0.945 s 
$ time for i in $(seq 10); do bin/PERFORMANCE_parser_urdf 2> /dev/null ; done
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (906 ms)
[----------] 1 test from URDFParser (906 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (906 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (894 ms)
[----------] 1 test from URDFParser (894 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (894 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (920 ms)
[----------] 1 test from URDFParser (920 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (920 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (892 ms)
[----------] 1 test from URDFParser (892 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (892 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (923 ms)
[----------] 1 test from URDFParser (923 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (923 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (921 ms)
[----------] 1 test from URDFParser (921 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (921 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (908 ms)
[----------] 1 test from URDFParser (908 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (908 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (945 ms)
[----------] 1 test from URDFParser (945 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (945 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (903 ms)
[----------] 1 test from URDFParser (903 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (903 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (890 ms)
[----------] 1 test from URDFParser (890 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (890 ms total)
[  PASSED  ] 1 test.

real	0m9.280s
user	0m6.680s
sys	0m2.490s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azeey azeey Awaiting requested review from azeey azeey is a code owner

@scpeters scpeters Awaiting requested review from scpeters scpeters is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
🏛️ ionic Gazebo Ionic
Projects
Core development
Status: Inbox
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arjo129 @scpeters